Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf: run installBcc in a subshell #4290

Merged
merged 10 commits into from
Apr 29, 2024
Merged

perf: run installBcc in a subshell #4290

merged 10 commits into from
Apr 29, 2024

Conversation

zachary-bailey
Copy link
Collaborator

@zachary-bailey zachary-bailey commented Apr 13, 2024 •
edited
Loading

What type of PR is this?

/kind performance

What this PR does / why we need it:

This PR modifies the way bcc tools are installed on ab VHDs. The installBcc function is ran in a subshell so that the main script can continue to execute while bcc packages are downloaded / compiled / installed in the background.

This PR results in about 3 minutes of saved time building the VHDs.

Which issue(s) this PR fixes:

Unsatisfactory VHD Build times.

Requirements:

@zachary-bailey zachary-bailey self-assigned this Apr 13, 2024
@coveralls
Copy link

coveralls commented Apr 13, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 8885833909

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.022%
Totals Coverage Status
Change from base Build 8883705751: 0.0%
Covered Lines: 2554
Relevant Lines: 3232
💛 - Coveralls

@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 45a23a8 to 452594e Compare April 15, 2024 14:13
@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 452594e to ef96350 Compare April 16, 2024 14:28
@ganeshkumarashok
Copy link
Contributor

What do you think about adding the tests for successful installation to this PR?

@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from ef96350 to aec92bb Compare April 17, 2024 19:30
@zachary-bailey
Copy link
Collaborator Author

What do you think about adding the tests for successful installation to this PR?

@ganeshkumarashok Can do!

@ganeshkumarashok
Copy link
Contributor

ganeshkumarashok commented Apr 18, 2024 •
edited
Loading

Could you note the approx time saved by parallelization, in the PR description as well?

@zachary-bailey
Copy link
Collaborator Author

zachary-bailey commented Apr 18, 2024 •
edited
Loading

Could you note the approx time saved by parallelization, in the PR description as well?

Do you mean time saved by running it in the subshell? or marking time for both, in the other PR as well? @ganeshkumarashok

ganeshkumarashok
ganeshkumarashok reviewed Apr 18, 2024
View reviewed changes
vhdbuilder/packer/install-dependencies.sh Outdated
- bcc-tools
- libbcc-examples
EOF
PARENT_DIR=$(pwd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be the present directory right? Not parent directory?

ganeshkumarashok
ganeshkumarashok reviewed Apr 18, 2024
View reviewed changes
vhdbuilder/packer/install-dependencies.sh Outdated
- libbcc-examples
EOF
PARENT_DIR=$(pwd)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a comment about creating a subshell to install in the background? This is not a common pattern in Agentbaker so it'll be good to note for the future

ganeshkumarashok
ganeshkumarashok reviewed Apr 18, 2024
View reviewed changes
vhdbuilder/packer/install-dependencies.sh Outdated
installBcc

exit $?
) > /tmp/bcc.log 2>&1 &
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/tmp directory may get deleted on reboots. Can you save it in /var/log/bcc_installation.log instead? Those logs will remain

@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 73343ab to 7042679 Compare April 23, 2024 15:02
@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 7042679 to 5d7a678 Compare April 24, 2024 14:57
@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 0905b59 to ce9d856 Compare April 25, 2024 14:36
ganeshkumarashok
ganeshkumarashok reviewed Apr 25, 2024
View reviewed changes
vhdbuilder/packer/test/linux-vhd-content-test.sh
echo "$line is missing"
result=1
fi
done
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified - just return 1 inside the then condition if line is missing

ganeshkumarashok
ganeshkumarashok reviewed Apr 25, 2024
View reviewed changes
vhdbuilder/packer/test/linux-vhd-content-test.sh Outdated
result=1
fi
done
if [ $result -eq 0 ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section can be removed and simplified

@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 9208a33 to 083362a Compare April 26, 2024 16:24
@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from 083362a to fa99b6e Compare April 29, 2024 17:25
@zachary-bailey zachary-bailey force-pushed the zacharybailey/installBccSubshell branch from fa99b6e to 5513985 Compare April 29, 2024 21:44
@zachary-bailey zachary-bailey merged commit 21b374d into master Apr 29, 2024
18 checks passed
@zachary-bailey zachary-bailey deleted the zacharybailey/installBccSubshell branch April 29, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ganeshkumarashok ganeshkumarashok ganeshkumarashok approved these changes

@juan-lee juan-lee Awaiting requested review from juan-lee juan-lee is a code owner

@cameronmeissner cameronmeissner Awaiting requested review from cameronmeissner cameronmeissner is a code owner

@UtheMan UtheMan Awaiting requested review from UtheMan UtheMan is a code owner

@anujmaheshwari1 anujmaheshwari1 Awaiting requested review from anujmaheshwari1 anujmaheshwari1 is a code owner

@AlisonB319 AlisonB319 Awaiting requested review from AlisonB319 AlisonB319 is a code owner

@Devinwong Devinwong Awaiting requested review from Devinwong Devinwong is a code owner

@lilypan26 lilypan26 Awaiting requested review from lilypan26 lilypan26 is a code owner

@ShiqianTao ShiqianTao Awaiting requested review from ShiqianTao

@AbelHu AbelHu Awaiting requested review from AbelHu AbelHu is a code owner

@junjiezhang1997 junjiezhang1997 Awaiting requested review from junjiezhang1997 junjiezhang1997 is a code owner

@jason1028kr jason1028kr Awaiting requested review from jason1028kr jason1028kr is a code owner

Assignees

@zachary-bailey zachary-bailey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zachary-bailey @coveralls @ganeshkumarashok